Disabling "orphaned" models when provider disabled#248
Disabling "orphaned" models when provider disabled#248
Conversation
When a provider is deselected on the Providers tab, models that would have zero enabled providers are now automatically removed from the selection. A confirmation dialog is shown to the user listing which models will be affected before proceeding. Changes: - Add computeModelsOnlyFromProvider() helper to identify orphaned models - Update toggleProviderEnabled() to remove orphaned models - Add DisableProviderConfirmDialog component - Update OrganizationProvidersAndModelsPage to show confirmation - Add comprehensive unit tests
…r-manager-improvements
Add logic to prevent enabling models when all their providers are disabled. When attempting to enable a model without any enabled providers, the model details dialog is opened instead.
…r-manager-improvements
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
src/components/organizations/providers-and-models/allowLists.domain.ts
Outdated
Show resolved
Hide resolved
src/components/organizations/providers-and-models/OrganizationProvidersAndModelsPage.tsx
Outdated
Show resolved
Hide resolved
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (6 files)
|
|
|
||
| // Collect all model IDs to check, expanding wildcards to concrete models | ||
| const modelIdsToCheck = new Set<string>(); | ||
| for (const entry of draftModelAllowList) { |
There was a problem hiding this comment.
WARNING: computeModelsOnlyFromProvider() returns [] when draftModelAllowList is empty ("allow all")
When draftModelAllowList.length === 0, modelIdsToCheck stays empty, so disabling a provider in "allow all models" mode will never surface orphaned models (and the confirm dialog won"t appear). If the intent is to warn/remove models that become unavailable, consider treating an empty allow list as "check all current models" (e.g., iterate modelProvidersIndex keys) before computing orphans.
| // If trying to enable a model, check if it has any enabled providers | ||
| if (nextAllowed) { | ||
| const providersForModel = selectors.modelProvidersIndex.get(modelId); | ||
| if (providersForModel) { |
There was a problem hiding this comment.
WARNING: Enabling a model with no providers may silently fail
providersForModel is only checked when truthy. If selectors.modelProvidersIndex.get(modelId) returns undefined (model missing from the index), enabling proceeds via actions.toggleModel, but later computeAllowedModelIds(..., enabledProviderSlugs) will currently drop models with unknown providers. Consider treating a missing index entry as "no enabled providers" (open details / block enable) to avoid a confusing toggle that doesn"t stick.
|
|
||
| const orphanedModels: string[] = []; | ||
|
|
||
| // Collect all model IDs to check, expanding wildcards to concrete models |
There was a problem hiding this comment.
WARNING: draftModelAllowList = [] ("all models allowed") yields no orphan detection
computeModelsOnlyFromProvider() only iterates draftModelAllowList, so when the list is empty it returns [] and the UI won’t warn that disabling a provider will implicitly disable models that are only served by that provider. Consider treating an empty allow list as "all currently-known models" (e.g. seed modelIdsToCheck from modelProvidersIndex.keys()) so the confirmation dialog is still shown when appropriate.
1 participant
Overview
This PR enhances the provider and model management page with better UX for handling provider-model relationships. It introduces confirmation dialogs when disabling providers would orphan models, and prevents users from enabling models that have no enabled providers.
Key Changes
When a user attempts to disable a provider that is the sole provider for one or more selected models, a confirmation dialog is now displayed showing:
When a user tries to enable a model that has no enabled providers: